[feat] User-initiated email change flow - #2830
Conversation
📝 WalkthroughWalkthroughThe account page now renders an Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| onClick={() => setShowChangePasswordModal(true)} | ||
| aria-label="Change password" | ||
| > | ||
| Change Password |
There was a problem hiding this comment.
"Change Password" -> "Change password". From looking at recent changes I think sentence case is the direction we're going in, though it's not completely consistent
|
@greptileai review @coderabbitai review |
|
The PR description images show the Account page on desktop and mobile, with the current email They also show the responsive change-email dialog: a centred desktop modal and a mobile bottom sheet. The flow visibly covers the initial form (“We’ll send a confirmation link…”), client-side validation for
✅ Action performedReview finished.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Greptile SummaryAdds a user-initiated email-change flow.
Confidence Score: 3/5This PR should not merge until concurrent email-change requests are prevented from bypassing the attempt limit. The rate-limit slot is recorded only after asynchronous availability checking and email delivery, allowing an authenticated user to issue an arbitrary concurrent burst that exceeds the intended three-attempt cap. Files Needing Attention: apps/website/src/server/routers/users.ts
|
| Filename | Overview |
|---|---|
| apps/website/src/server/routers/users.ts | Adds the self-service procedure and rate limiter, but concurrent requests bypass the intended attempt cap. |
| apps/website/src/components/settings/ChangeEmailModal.tsx | Adds the modal’s validation, submission, loading, conflict, error, and success states. |
| apps/website/src/lib/api/customerio.ts | Adds the escaped transactional courtesy notice sent to the current email address. |
| apps/website/src/components/settings/AccountSettingsSection.tsx | Adds the change-email entry point and incorporates it alongside password settings. |
| apps/website/src/lib/schemas/user/changeEmail.schema.ts | Centralizes trimming, lowercasing, and validation of prospective email addresses. |
Sequence Diagram
sequenceDiagram
participant U as Authenticated user
participant A as requestOwnEmailChange
participant M as Attempt map
participant E as Availability/email services
par Concurrent request 1
U->>A: Request email change
A->>M: "Check attempts (< 3)"
A->>E: Await availability and delivery
and Concurrent request N
U->>A: Request email change
A->>M: "Check same unchanged attempts (< 3)"
A->>E: Await availability and delivery
end
E-->>A: Complete
A->>M: Record attempts after side effects
Reviews (2): Last reviewed commit: "[style] Drop the nothing-changes line fr..." | Re-trigger Greptile
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/website/src/components/settings/AccountSettingsSection.tsx (1)
31-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the narrating
what-comments.
{/* Account Settings Section */}and{/* Change Email Modal */}just restate what the adjacent JSX already makes obvious.As per coding guidelines: "Write comments only when the reason is non-obvious... and remove obvious or outdated comments during refactoring."
♻️ Proposed cleanup
- {/* Account Settings Section */} <div className="mb-6">- {/* Change Email Modal */} <ChangeEmailModalAlso applies to: 61-61
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/website/src/components/settings/AccountSettingsSection.tsx` at line 31, Remove the redundant JSX comments `{/* Account Settings Section */}` and `{/* Change Email Modal */}` from the component, leaving the surrounding JSX structure and behavior unchanged.Source: Coding guidelines
apps/website/src/components/settings/ChangeEmailModal.tsx (1)
1-166: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing Storybook story for a new user-facing component.
ChangeEmailModalis a new, standalone user-facing component but no accompanying Storybook story is included in this file set.As per coding guidelines: "Add a Storybook story alongside every new user-facing component and keep stories synchronized when components are added, modified, or removed." Please confirm a story exists elsewhere, or add one.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/website/src/components/settings/ChangeEmailModal.tsx` around lines 1 - 166, Add a Storybook story for the standalone ChangeEmailModal component, or confirm and wire up an existing story if one already exists elsewhere. Cover the open modal state and provide the required isOpen and setIsOpen props, keeping the story synchronized with the component’s current user-facing behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/website/src/server/routers/users.ts`:
- Around line 35-55: Update sendEmailChangeConfirmation to pass the normalised
current email as oldEmail when calling createEmailChangeToken, matching the
value later validated by confirmEmailChange. Add coverage for a stored email
containing mixed case or whitespace, ensuring the confirmation flow remains
valid.
---
Nitpick comments:
In `@apps/website/src/components/settings/AccountSettingsSection.tsx`:
- Line 31: Remove the redundant JSX comments `{/* Account Settings Section */}`
and `{/* Change Email Modal */}` from the component, leaving the surrounding JSX
structure and behavior unchanged.
In `@apps/website/src/components/settings/ChangeEmailModal.tsx`:
- Around line 1-166: Add a Storybook story for the standalone ChangeEmailModal
component, or confirm and wire up an existing story if one already exists
elsewhere. Cover the open modal state and provide the required isOpen and
setIsOpen props, keeping the story synchronized with the component’s current
user-facing behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ea6b4e82-56db-4cfb-ae0e-cdd596d61f42
⛔ Files ignored due to path filters (1)
apps/website/src/__tests__/pages/__snapshots__/account.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (7)
apps/website/src/components/settings/AccountSettingsSection.test.tsxapps/website/src/components/settings/AccountSettingsSection.tsxapps/website/src/components/settings/ChangeEmailModal.test.tsxapps/website/src/components/settings/ChangeEmailModal.tsxapps/website/src/pages/account.tsxapps/website/src/server/routers/users.test.tsapps/website/src/server/routers/users.ts
joshestein
left a comment
joshestein
left a comment
There was a problem hiding this comment.
For changing password, the user currently needs to validate their old password. I understand we can't validate a past user email in the same way, but maybe for user-initiated email changes it's worth sending a courtesy email to their old email as a form of 'validation'?
I don't think there's real risk to accounts being taken over but might still be worth letting the user know.
| requestEmailChange.mutate({ newEmail: result.data }); | ||
| }; | ||
|
|
||
| const handleKeyDown = (e: React.KeyboardEvent) => { |
There was a problem hiding this comment.
If we use a semantic <form> do we still need this?
There was a problem hiding this comment.
Nope, <form> is much better. I've changed it!
This is a good idea 👍, I'll add this |
86f0ea9 to
7cefdf6
Compare
…l change is requested
marn-in-prod
left a comment
marn-in-prod
left a comment
There was a problem hiding this comment.
Getting an email change notification sent to the original email would be good + not counting the user towards usage if our own infra failed. The others are mainly consistency things
…ward the rate limit
Render
Destroyed
Render
Destroyed
Render
Destroyed
|
Here's the new courtesy email for reference: |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/website/src/server/routers/users.ts (1)
91-106: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBound the Keycloak cleanup retries or move them off the confirm-email path.
unlinkStaleGoogleIdentitiesOrAlertis awaited inconfirmEmailChange, and it callsunlinkStaleGoogleIdentities3 times sequentially.adminRequestusesaxios.request(...)without atimeout; Axios request timeout defaults to0, so a slow/unresponsive Keycloak admin client can make the user’s confirm link hang indefinitely. Since stale Google identity cleanup is best-effort, cap the total retry wall-clock time or make the cleanup fire-and-forget.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/website/src/server/routers/users.ts` around lines 91 - 106, Update unlinkStaleGoogleIdentitiesOrAlert and its confirmEmailChange call path so stale Google identity cleanup cannot block indefinitely: add a bounded timeout covering each Keycloak admin request and the retry sequence, or dispatch the best-effort cleanup without awaiting it. Preserve the existing retry and slack-alert behavior for failures while ensuring confirmEmailChange completes even when Keycloak is slow or unresponsive.
🧹 Nitpick comments (4)
apps/website/src/components/settings/ChangeEmailModal.tsx (2)
60-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated fixed-width modal spacer hack. Both modals use an invisible
<div className="h-0 w-[600px] max-w-full" />purely to force the modal's rendered width; the same effect can be achieved directly withmin-w-[600px] max-w-fullon the content wrapper, which also better matches the guideline of preferring minimum over fixed dimensions.
apps/website/src/components/settings/ChangeEmailModal.tsx#L60-L61: replace the spacer div withmin-w-[600px] max-w-fullon the<div className="w-full max-w-modal">wrapper (or a shared modal-width helper).apps/website/src/components/settings/AccountSettingsSection.tsx#L192-L193: apply the same change toChangePasswordModal's wrapper, ideally sharing one utility/class between both modals to prevent the two widths drifting apart over time.As per coding guidelines: "Prefer
min-h-[Xpx]overh-[Xpx], use minimum dimensions for flexibility, and avoid maximum dimensions exceptmax-w-prose."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/website/src/components/settings/ChangeEmailModal.tsx` around lines 60 - 61, The modal width is forced by duplicated invisible fixed-width spacer elements. In apps/website/src/components/settings/ChangeEmailModal.tsx lines 60-61, move the 600px minimum width onto the content wrapper and remove the spacer; apply the same change to ChangePasswordModal in apps/website/src/components/settings/AccountSettingsSection.tsx lines 192-193, preferably through a shared utility/class, while preserving responsive full-width behavior.Source: Coding guidelines
1-154: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a Storybook story for
ChangeEmailModal.
ChangeEmailModalis a new user-facing component, but no matching story file is present underapps/website/src/components/settings.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/website/src/components/settings/ChangeEmailModal.tsx` around lines 1 - 154, Add a Storybook story file for the ChangeEmailModal component under the settings components stories, configuring the required isOpen and setIsOpen props and providing a representative default interaction state.Source: Coding guidelines
apps/website/src/lib/api/customerio.ts (1)
130-177: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the shared
fetch/error-handling boilerplate.
sendEmailChangeVerificationandsendEmailChangeRequestedNoticeduplicate the same POST-to-/send/emailrequest/response handling, differing only into,subject, andbody. A small shared helper (e.g.postTransactionalEmail({ to, identifierEmail, subject, body })) would reduce duplication and keep future error-handling changes in one place.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/website/src/lib/api/customerio.ts` around lines 130 - 177, The duplicated Customer.io POST and response handling in sendEmailChangeVerification and sendEmailChangeRequestedNotice should be extracted into a shared helper such as postTransactionalEmail accepting recipient, identifier email, subject, and body. Update both functions to build their message-specific values and delegate the request and HTTP error handling to that helper, preserving their existing payloads and error behavior.apps/website/src/server/routers/users.ts (1)
35-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
envfor the confirmation URL base.
apps/website/src/server/routers/users.ts:36still readsprocess.env.NEXT_PUBLIC_SITE_URLdirectly, while this file already goes throughenvfor server-side values. ExposeNEXT_PUBLIC_SITE_URLviaenvor add a small server-side config helper so this helper does not bypass central env handling.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/website/src/server/routers/users.ts` around lines 35 - 38, Update getEmailChangeConfirmUrl to obtain the confirmation URL base through the existing env configuration rather than reading process.env.NEXT_PUBLIC_SITE_URL directly. Expose NEXT_PUBLIC_SITE_URL through env or reuse a small server-side configuration helper, while preserving the current default URL and token encoding behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/website/src/server/routers/users.ts`:
- Around line 60-87: Replace the process-local emailChangeAttemptsByUserId Map
used by assertWithinEmailChangeRateLimit, recordEmailChangeAttempt, and
resetEmailChangeRateLimits with a shared persistent store such as Redis or a
database-backed record with expiration. Ensure checks and recording are atomic
across instances, prune expired attempts independently of new checks, and
preserve the existing 3-attempts-per-30-minute behavior and error response.
---
Outside diff comments:
In `@apps/website/src/server/routers/users.ts`:
- Around line 91-106: Update unlinkStaleGoogleIdentitiesOrAlert and its
confirmEmailChange call path so stale Google identity cleanup cannot block
indefinitely: add a bounded timeout covering each Keycloak admin request and the
retry sequence, or dispatch the best-effort cleanup without awaiting it.
Preserve the existing retry and slack-alert behavior for failures while ensuring
confirmEmailChange completes even when Keycloak is slow or unresponsive.
---
Nitpick comments:
In `@apps/website/src/components/settings/ChangeEmailModal.tsx`:
- Around line 60-61: The modal width is forced by duplicated invisible
fixed-width spacer elements. In
apps/website/src/components/settings/ChangeEmailModal.tsx lines 60-61, move the
600px minimum width onto the content wrapper and remove the spacer; apply the
same change to ChangePasswordModal in
apps/website/src/components/settings/AccountSettingsSection.tsx lines 192-193,
preferably through a shared utility/class, while preserving responsive
full-width behavior.
- Around line 1-154: Add a Storybook story file for the ChangeEmailModal
component under the settings components stories, configuring the required isOpen
and setIsOpen props and providing a representative default interaction state.
In `@apps/website/src/lib/api/customerio.ts`:
- Around line 130-177: The duplicated Customer.io POST and response handling in
sendEmailChangeVerification and sendEmailChangeRequestedNotice should be
extracted into a shared helper such as postTransactionalEmail accepting
recipient, identifier email, subject, and body. Update both functions to build
their message-specific values and delegate the request and HTTP error handling
to that helper, preserving their existing payloads and error behavior.
In `@apps/website/src/server/routers/users.ts`:
- Around line 35-38: Update getEmailChangeConfirmUrl to obtain the confirmation
URL base through the existing env configuration rather than reading
process.env.NEXT_PUBLIC_SITE_URL directly. Expose NEXT_PUBLIC_SITE_URL through
env or reuse a small server-side configuration helper, while preserving the
current default URL and token encoding behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: fd1ac857-e35e-44dd-9079-82b2e3bba55c
⛔ Files ignored due to path filters (1)
apps/website/src/__tests__/pages/__snapshots__/account.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (10)
apps/website/src/components/settings/AccountSettingsSection.test.tsxapps/website/src/components/settings/AccountSettingsSection.tsxapps/website/src/components/settings/ChangeEmailModal.test.tsxapps/website/src/components/settings/ChangeEmailModal.tsxapps/website/src/lib/api/customerio.test.tsapps/website/src/lib/api/customerio.tsapps/website/src/lib/schemas/user/changeEmail.schema.tsapps/website/src/pages/account.tsxapps/website/src/server/routers/users.test.tsapps/website/src/server/routers/users.ts
Looks good, but seperately, the email addressed attached to email changes isn't a noreply email address right? It's one where Bluedot folks can easily see messages from? I'm asking because the last time I reset my password, the email came from "noreply@bluedot.org" |
It uses team@bluedot.org, which is the general contact email we use. I also just sent an email to check that it's monitored. I won't wait for that before shipping though, it can be fixed after the fact if not. |
3 participants
Description
Builds on the admin-initiated version of the change-email flow (#2828), the code here should be relatively self explanatory given that. The UI is the more important thing to focus on in review.
Issue
Fixes #2570
Developer checklist
Screenshot
Account page & change email modal
Confirm page
These screenshots are actually taken from #2828.